Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cache): add staleMaxAge option for caching header #116

Merged
merged 7 commits into from
Apr 19, 2022
Merged

feat(cache): add staleMaxAge option for caching header #116

merged 7 commits into from
Apr 19, 2022

Conversation

Intevel
Copy link
Contributor

@Intevel Intevel commented Apr 15, 2022

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control
stale-while-revalidate gives you the option to revalidate the response in the background after a certain time.

Example:
{ swr: true, name: "mySWRData", magAge: 1, staleMaxAge: 30 }

Will create this cache-control headers:
cache-control: s-maxage=1, stale-while-revalidate=30

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@Intevel Intevel changed the title feat(cache): add options for swr max-age c07da5b feat(cache): add options for swr max-age Apr 15, 2022
@@ -135,7 +136,11 @@ export function defineCachedEventHandler (handler: CompatibilityEventHandler, op
if (opts.magAge) {
cacheControl.push(`s-maxage=${opts.magAge}`)
}
cacheControl.push('stale-while-revalidate')
if (opts.staleMaxAge) {
cacheControl.push(`stale-while-revalidate=${opts.staleMaxAge}`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking since we are keeping stale value up to maxAge anyway does it make sense to default it to opts.maxAge?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly you don't use the same values. I think leaving multiple options open to a developer is better.

Copy link
Member

@pi0 pi0 Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, however, this is the actual behavior of cachify function right now. We only revalidate until maxAge reaches at least not before (always revalidate)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but that is not the point behind swr

Copy link
Member

@pi0 pi0 Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stale-while-revalidate header alone tells browser/client always revalidate no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but with the options you can define when it should revalidate and not only when the response expired.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know but we already know that data is not going to be invalidated until maxAge. But really we can improve default generated value later ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have convinced me.

@pi0 pi0 changed the title feat(cache): add options for swr max-age feat(cache): add staleMaxAge option for caching header Apr 15, 2022
@Intevel Intevel marked this pull request as draft April 15, 2022 19:54
@pi0 pi0 marked this pull request as ready for review April 19, 2022 08:35
@pi0
Copy link
Member

pi0 commented Apr 19, 2022

Let's iterate with next PRs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants